Skip to content

Handle return value of JS_ToFloat64 #891

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 7, 2025
Merged

Handle return value of JS_ToFloat64 #891

merged 1 commit into from
Feb 7, 2025

Conversation

saghul
Copy link
Contributor

@saghul saghul commented Feb 6, 2025

Fixes: #890

@saghul saghul mentioned this pull request Feb 6, 2025
quickjs.c Outdated
Comment on lines 6768 to 6770
if (JS_ToFloat64(ctx, &d, ctx->error_stack_trace_limit) < 0) {
// Ignore error since it sets d to NAN anyway.
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better:

Suggested change
if (JS_ToFloat64(ctx, &d, ctx->error_stack_trace_limit) < 0) {
// Ignore error since it sets d to NAN anyway.
}
// Ignore error since it sets d to NAN anyway.
// coverity[check_return]
JS_ToFloat64(ctx, &d, ctx->error_stack_trace_limit);

Clearer and, from what I remember from node's coverity scans, "using" the value like you do here doesn't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that works, great!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trufae how could be test this?

@saghul saghul force-pushed the handle-result-conv branch from 228596f to 863a6bd Compare February 7, 2025 10:03
@saghul
Copy link
Contributor Author

saghul commented Feb 7, 2025

A quick github search suggests that is indeed the right way to silence it. Updated!

@trufae
Copy link
Contributor

trufae commented Feb 7, 2025

Looks good to me

@chqrlie
Copy link
Collaborator

chqrlie commented Feb 7, 2025

In case an exception is thrown by JS_ToFloat64 does the pending exception ever pose a problem? Can this conversion fail? Shouldn't we clear the exception if thrown?

@saghul
Copy link
Contributor Author

saghul commented Feb 7, 2025

In case an exception is thrown by JS_ToFloat64 does the pending exception ever pose a problem? Can this conversion fail? Shouldn't we clear the exception if shown?

We restore the exception right after the dance, so it will be discarded.

@saghul saghul merged commit 882f885 into master Feb 7, 2025
59 checks passed
@saghul saghul deleted the handle-result-conv branch February 7, 2025 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coverity scan
4 participants